Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to Customize HAProxy 2.x Error Page #680

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

miheer
Copy link
Contributor

@miheer miheer commented Mar 7, 2021

Ability to Customize HAProxy 2.x Error Page
https://issues.redhat.com/browse/NE-379

@miheer miheer force-pushed the customize-pages branch 6 times, most recently from b70c0e1 to 9105125 Compare March 7, 2021 14:18
@miheer
Copy link
Contributor Author

miheer commented Mar 7, 2021

@Miciah PTAL

Customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711).
does not work with OCP 4. It's also important that the right page as per user requirements is served at the right moment.
Example - 503 error page shall be returned if no pod is available.
But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements.

How does this enhancement accomplish this? Can multiple error pages (ie one HTML page for 503, and one HTML page for 404) be appended to the same error pages config map (and thus fed into HAProxy)? I would suggest elaborating on how a user specifics multiple error pages somewhere in this enhancement (it's not entirely clear to me).

Copy link
Contributor Author

@miheer miheer Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configmap can have multiple pages

## Summary
There is no supported and reasonable method to customize the HAProxy 2.x error page as deployed in OpenShift.
Customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711).
does not work with OCP 4. It's also important that the right page as per user requirements is served at the right moment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711). does not work with OCP 4

This solution was never intended to work on OCP 4, right? Also, I would suggest removing the spurious period after the parenthesis.

Copy link
Contributor Author

@miheer miheer Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will reword it. Yes it is not meant for OCP 4. Customers say 503 page is served even when the page is not found when it shall serve 404. The reason I think that happens is because we provide only 503 in our haproxy template

But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements.
## Motivation
In OCP 3.11 customizing error pages is possible via a [documented procedure](https://access.redhat.com/solutions/3425711)
Customers resist customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Customers resist customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711)

Are you saying that customers in OCP 3.11 resist following the recommended procedure? If so, why? Or are you saying customers on OCP 4.x resist following this procedure for updating the HAProxy error pages (simply because it does not work)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that's a different thing. Customers say 503 page is served even when the page is not found when it shall serve 404. The reason I think that happens is because we provide only 503 in our haproxy template

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make it clear

1. The IngressController API is extended by adding an optional
`HttpErrorCodePage` field with type string
```go
//This used to sync the configmap having customizable error code page created by cluster admin in openshift-config namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I would suggest using a single space after each comment indicator, ie // <text> instead of //<text>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

```
2. `errorfile` stanzas need to be added as per https://www.haproxy.com/blog/serve-dynamic-custom-error-pages-with-haproxy/
3. default error pages will be added in openshift/router for the haproxy.
4. A new controller to sync the cluster admin created configmap having custom error pages from `openshift-config`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is syncing the config map from the openshift-config namespace to the openshift-ingress namespace necessary? Is there a reason we can't just use the config map in the original namespace directly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the standard pattern for operators: User-created configmaps and secrets go in openshift-config, operator-managed shared configmaps and secrets go in openshift-config-managed, and operators synch configmaps and secrets from those namespaces to their own namespaces as needed. This eases auditing and provides more control over access, validation, and updates. Unfortunately we didn't follow this pattern in the ingress operator from the start with some resources, but it is the pattern we're trying to follow in all operators.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, thanks for the historical explanation. Thanks!

3. default error pages will be added in openshift/router for the haproxy.
4. A new controller to sync the cluster admin created configmap having custom error pages from `openshift-config`
to `openshift-ingress`.
5. changes to ingress controller's function `desiredRouterDeployment` present in `deployment.go`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
5. changes to ingress controller's function `desiredRouterDeployment` present in `deployment.go`.
5. changes to ingress controller's function `desiredRouterDeployment` present in `deployment.go`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK


### Validation
Omitting `spec.httpErrorCodePage` specifies the default behavior i.e serves default error code pages.
If user specifies an invalid value, then the IngressController ignores the annotation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user specifies an invalid value, then the IngressController ignores the annotation.

What constitutes an invalid value? Which annotation are you referring to here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, what keys does the configmap need to have? What is the format of the data values? For example, does an error page need to include the HTTP header? (Presumably it should.) Does the page need to have an HTML document? (I expect text/plain would work.) If HTML, is it validated? (I don't think we should validate HTML, but we should call out the design decision.) Is there a max length? Does encoding matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet decided on the max length of the page. Need to think on encoding.

router pods.

### Risks and Mitigations
No risk identified. This is a cosmetic change.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a couple risks worth mentioning.

  1. What if a user supplies an incomplete or confusing error page(s)? Or what if a user provides an absolutely massive HTML file for a trivial 503? Could that effect performance somehow?
  2. Are there any applications that use the default ingress controller that expect a 503 in non-503 cases? Does the administrator need to exercise caution when creating custom error pages for the default ingress controller? (I would hope not, but it might be worth double checking)

Copy link
Contributor Author

@miheer miheer Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user does not provide proper file name in configmap then the custom error page will not be served. The default shall be served in that case

Yeah admin will need to take some precaution. I will list them. May be we can do some validations at our end. Currently if page is not found then also 503 is served. This I think is because we have only 503 page defined in errorfile section of haproxy template of the router.

Unit test cases will be added for the new controller which sync the configmap from openshift-config
to openshift-ingress.
Unit test cases will be added for the ingress controller where the router deployment get a volume mount
of the custom error code page
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggestion adding some details about a potential e2e test for this enhancement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

namespace: openshift-ingress-operator
spec:
httpErrorCodePage: "http503page" // http503page is the config map name with custom
//503 error page created in openshift-config by cluster-admin.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yaml comments start with #. But instead of comments in the yaml, please provide an example definition for the "http503page" configmap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@miheer miheer force-pushed the customize-pages branch 2 times, most recently from 8e69340 to 99fbb67 Compare March 26, 2021 01:53
@Miciah
Copy link
Contributor

Miciah commented Mar 26, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2021
@Miciah
Copy link
Contributor

Miciah commented Mar 26, 2021

/lgtm cancel
Sorry, wrong PR.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2021
@miheer miheer force-pushed the customize-pages branch 3 times, most recently from 1010de8 to e18070c Compare March 26, 2021 02:14
Comment on lines 28 to 31
There is no supported and reasonable method to customize the HAProxy 2.x error page in OCP 4.x
It's also important that the right page as per user requirements is served at the right moment.
Example - 503 error page shall be returned if no pod is available.
But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
There is no supported and reasonable method to customize the HAProxy 2.x error page in OCP 4.x
It's also important that the right page as per user requirements is served at the right moment.
Example - 503 error page shall be returned if no pod is available.
But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements.
There is no supported method to customize an IngressController's error pages in OCP 4.
Users may want to customize an IngressController's error pages for branding or other reasons.
For example, users may want a custom HTTP 503 error page to be returned if no pod is available.
When the requested URI does not exist, users may want an IngressController to return a custom 404 page.

Comment on lines 32 to 33
Using this feature, user can provide custom http pages for 503 and 404 for the haproxy router by adding these pages
in a configmap using --file attribute.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the "--file attribute"? I'm guessing the phrase can be deleted.

Suggested change
Using this feature, user can provide custom http pages for 503 and 404 for the haproxy router by adding these pages
in a configmap using --file attribute.
Using this feature, users can provide custom HTTP pages for HTTP 503 and 404 responses using a configmap.

Users say 503 page is served even when the page is not found when it shall serve 404.
The reason that happens is because we provide only 503 in our haproxy template.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this a secondary motivation for this enhancement. The primary motivation is that users want to customize the error pages that are returned, for example for branding purposes. Please mention both motivations.

The reason that happens is because we provide only 503 in our haproxy template.

### Goals
1. Enable the cluster administrator to specify custom error code pages for haproxy router to serve them at the right moment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the motivation and summary, it seems you can also add this goal: "Return a distinct HTTP 404 error page when the requested URI does not exist, instead of returning the HTTP 503 error page."

I think you can delete the phrase "at the right moment". If the phrase refers to returning 404s, then that should be a separate goal. If the phrase means something else, please clarify.

1. Enable the cluster administrator to specify custom error code pages for haproxy router to serve them at the right moment.

### Non-Goal
TBD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Custom error pages for HTTP responses other than 503 and 404.
  • Enabling users to configure custom error pages for individual routes.

Comment on lines 116 to 115
1. If a user provides incorrect filename than specified format i.e error-page-<error code>.http in
the keys of the configmap then it will be ignored and default page will be served.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. If a user provides incorrect filename than specified format i.e error-page-<error code>.http in
the keys of the configmap then it will be ignored and default page will be served.
1. If a user provides a key that is not of the format `error-page-<error code>.http` in
the configmap, then the configmap will be ignored, and default error pages will be served.

Comment on lines 123 to 122
Unit test cases will be added for the new controller which sync the configmap from openshift-config
to openshift-ingress.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Unit test cases will be added for the new controller which sync the configmap from openshift-config
to openshift-ingress.
Unit test cases will be added for the new controller which sync the configmap from `openshift-config`
to `openshift-ingress`.

Comment on lines 140 to 139
1. https://github.com/openshift/api/pull/843 - Adds an field called `httpErrorCodePages` to the ingress controller API to get the custom error page configmap created
in openshift-config namespace which is then synced to openshift-ingress namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. https://github.com/openshift/api/pull/843 - Adds an field called `httpErrorCodePages` to the ingress controller API to get the custom error page configmap created
in openshift-config namespace which is then synced to openshift-ingress namespace.
1. https://github.com/openshift/api/pull/843 - Adds a field called `httpErrorCodePages` to the ingress controller API to get the custom error page configmap created
in the `openshift-config` namespace which is then synced to `openshift-ingress` namespace.

1. https://github.com/openshift/api/pull/843 - Adds an field called `httpErrorCodePages` to the ingress controller API to get the custom error page configmap created
in openshift-config namespace which is then synced to openshift-ingress namespace.
2. https://github.com/openshift/cluster-ingress-operator/pull/571
a. Controller to sync configmaps from openshift config namespace to openshift-ingress namespace NE-535
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
a. Controller to sync configmaps from openshift config namespace to openshift-ingress namespace NE-535
a. Controller to sync configmaps from the `openshift-config` namespace to the `openshift-ingress` namespace ([NE-535](https://issues.redhat.com/browse/NE-535)).

Is NE-535 specifically for this part of the feature?

// Currently only error pages for 503 and 404 responses can be customized.
// Each value in the configmap should be the full response, including HTTP headers.
// If this field is empty, the ingress controller uses the default error pages.
HttpErrorCodePages string `json:"httpErrorCodePages,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be the configmap reference type?

Suggested change
HttpErrorCodePages string `json:"httpErrorCodePages,omitempty"`
HttpErrorCodePages configv1.ConfigMapNameReference `json:"httpErrorCodePages,omitempty"`

@miheer miheer force-pushed the customize-pages branch 7 times, most recently from 26a17ee to d794df2 Compare March 26, 2021 08:27
@miheer
Copy link
Contributor Author

miheer commented Mar 26, 2021

@Miciah PTAL

Copy link
Contributor

@Miciah Miciah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still some unaddressed issues. However, we need to unblock the openshift/api PR. Can you do https://github.com/openshift/enhancements/pull/680/files#r601968021 for now? We can get the rest in follow-ups.

The reason that happens is because we provide only 503 in our haproxy template.

### Goals
Return a distinct HTTP 404 error page when the requested URI does not exist, instead of returning the HTTP 503 error page.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you accidentally deleted the old goal: "Enable the cluster administrator to specify custom error code pages for haproxy router".

Comment on lines 41 to 42
Custom error pages for HTTP responses other than 503 and 404.
Enabling users to configure custom error pages for individual routes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use bullets.

Comment on lines 72 to 73
2. The configmap keys shall have the format error-page-`<error code>`.http. Right now only error-page-503.http and
error-page-404.http will be available.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
2. The configmap keys shall have the format error-page-`<error code>`.http. Right now only error-page-503.http and
error-page-404.http will be available.
2. The configmap keys shall have the format `error-page-<error code>.http`. Right now only `error-page-503.http` and
`error-page-404.http` will be recognized.

Comment on lines 75 to 77
$ oc -n openshift-config create configmap my-custom-error-code-pages\
--from-file=error-page-503.http\
--from-file=error-page-404.http
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ oc -n openshift-config create configmap my-custom-error-code-pages\
--from-file=error-page-503.http\
--from-file=error-page-404.http
$ oc -n openshift-config create configmap my-custom-error-code-pages \
--from-file=error-page-503.http \
--from-file=error-page-404.http

Or unwrap it:

Suggested change
$ oc -n openshift-config create configmap my-custom-error-code-pages\
--from-file=error-page-503.http\
--from-file=error-page-404.http
$ oc -n openshift-config create configmap my-custom-error-code-pages --from-file=error-page-503.http --from-file=error-page-404.http

Comment on lines 83 to 86
error-page-503.http: |-
<your custom http response page contents>
error-page-404.http: |
<your custom http response page contents>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to be done.

// If this field is empty, the ingress controller uses the default error pages.
HttpErrorCodePages string `json:"httpErrorCodePages,omitempty"`
```
Using the default, the router uses the default error pages used by haproxy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to be fixed.


```
3. `errorfile` stanzas need to be added as per https://www.haproxy.com/blog/serve-dynamic-custom-error-pages-with-haproxy/
4. default error pages will be added in openshift/router for the haproxy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to be addressed. I think you mean to say the following:

Suggested change
4. default error pages will be added in openshift/router for the haproxy.
4. A default error page for HTTP 404 responses will be added in openshift/router.

4. default error pages will be added in openshift/router for the haproxy.
5. A new controller to sync the cluster admin created configmap having custom error pages from `openshift-config`
to `openshift-ingress`.
6. changes to ingress controller's function `desiredRouterDeployment` present in `deployment.go` to mount the configmap to the router.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still should be fixed.

### User Stories

#### As a cluster administrator, I expect to have custom error code pages getting served
1. The administrator creates a configmap named http503page in the openshift-config namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still isn't addressed.

Comment on lines 109 to 110
3. The ingress operator copies the http503page configmap from the openshift-config namespace to the
openshift-ingress namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still isn't addressed.

@miheer miheer force-pushed the customize-pages branch 2 times, most recently from e77e05d to 90b9473 Compare March 30, 2021 01:32
@Miciah
Copy link
Contributor

Miciah commented Mar 30, 2021

Thanks! The enhancement looks well defined in its current form, modulo some markup issues and such. We can clean up the remaining issues later, but let's get this PR merged to unblock openshift/api#843.
/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2021
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2021
@Miciah
Copy link
Contributor

Miciah commented Mar 30, 2021

Thanks! Maybe the linter will tolerate the simplified example...
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit e3450f2 into openshift:master Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants